Skip to content

Add Ktor client integration #4527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 34 commits into
base: main
Choose a base branch
from
Open

Add Ktor client integration #4527

wants to merge 34 commits into from

Conversation

lcian
Copy link
Member

@lcian lcian commented Jun 27, 2025 •
edited
Loading

📜 Description

Add a Ktor client integration.
Functionality, options and tests are similar to our OkHttp integration.

💡 Motivation and Context

Part of #2684

💚 How did you test it?

End to end and manual tests

📝 Checklist

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

  • Changelog
  • Docs
  • Readme
  • Add module to main readme
  • Release
  • Add module to .craft.yml
  • Add module to release registry
  • e2e tests

lcian
lcian commented Jun 27, 2025
View reviewed changes
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 27, 2025 •
edited
Loading

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 467.44 ms 488.58 ms 21.14 ms
Size 1.58 MiB 2.09 MiB 519.50 KiB

Previous results on branch: lcian/feat/ktor-client

Startup times

Revision Plain With Sentry Diff
569c88a 406.86 ms 419.57 ms 12.72 ms
bc4c3ab 420.27 ms 462.92 ms 42.65 ms
94e993e 404.49 ms 439.62 ms 35.13 ms
df1fbd1 407.84 ms 424.93 ms 17.09 ms
b5f9205 492.51 ms 542.98 ms 50.47 ms
ec32652 354.67 ms 385.71 ms 31.04 ms
1df63ce 429.52 ms 485.02 ms 55.50 ms
1fa99ff 371.94 ms 406.67 ms 34.74 ms
c3b3ac8 433.74 ms 490.08 ms 56.34 ms
f316c06 427.39 ms 464.94 ms 37.55 ms

App size

Revision Plain With Sentry Diff
569c88a 1.58 MiB 2.09 MiB 519.08 KiB
bc4c3ab 1.58 MiB 2.09 MiB 518.90 KiB
94e993e 1.58 MiB 2.09 MiB 519.31 KiB
df1fbd1 1.58 MiB 2.09 MiB 519.08 KiB
b5f9205 1.58 MiB 2.09 MiB 519.31 KiB
ec32652 1.58 MiB 2.09 MiB 518.89 KiB
1df63ce 1.58 MiB 2.09 MiB 518.90 KiB
1fa99ff 1.58 MiB 2.09 MiB 519.27 KiB
c3b3ac8 1.58 MiB 2.09 MiB 518.90 KiB
f316c06 1.58 MiB 2.09 MiB 519.08 KiB

@lcian lcian changed the title [WIP] Add Ktor client integration \Add Ktor client integration Jul 3, 2025
@lcian lcian changed the title \Add Ktor client integration Add Ktor client integration Jul 3, 2025
@lcian lcian marked this pull request as ready for review July 3, 2025 10:55
romtsn
romtsn reviewed Jul 14, 2025
View reviewed changes
romtsn
romtsn reviewed Jul 14, 2025
View reviewed changes
@lcian
Copy link
Member Author

lcian commented Jul 15, 2025

LGTM, but there's one caveat we have to consider: If someone uses ktor with the OkHttp engine, we'd be potentially double-instrumenting the http calls, especially with SAGP in place. Could be a follow up PR, but I guess we have to add some protection against that case (like checking if engine == OkHttp and platform == Android then don't instrument OR retrieve the list of interceptors/eventListner from OkHttp client if possible, and check if ours are already there)

Good call. Working on this and trying to keep the extra dependencies at a minimum. I think we'll need to bring in the library containing the OkHttp client engine at least, which probably also depends on OkHttp itself.

@lcian
Copy link
Member Author

lcian commented Jul 16, 2025

@sentry review

@seer-by-sentry Seer by Sentry
Copy link

PR Description

This pull request introduces a new integration for the Sentry error monitoring platform with the Ktor HTTP client. The goal is to automatically capture unhandled exceptions, create breadcrumbs for HTTP requests, and support distributed tracing for applications using Ktor's client.

Click to see more

Key Technical Changes

The key technical changes include: 1. Creation of a SentryKtorClientPlugin that implements Ktor's ClientPlugin interface. 2. Implementation of request and response interceptors to create spans and breadcrumbs. 3. Addition of a SentryKtorClientUtils class to handle error capturing and breadcrumb creation. 4. Implementation of SentryKtorClientPluginContextHook to manage scopes using SentryContext. 5. Addition of tests to verify the plugin's functionality.

Architecture Decisions

The architectural decisions include: 1. Using Ktor's ClientPlugin API to integrate with the HTTP client pipeline. 2. Using Sentry's existing IScopes and ISpan interfaces for managing scope and spans. 3. Implementing a context hook to ensure proper scope management within the Ktor pipeline. 4. Providing configuration options for capturing failed requests, defining target URLs, and customizing spans.

Dependencies and Interactions

This integration depends on the Ktor HTTP client library, the Sentry Java SDK, and the kotlinx.coroutines library. It interacts with the Sentry SDK to capture exceptions, create breadcrumbs, and manage spans. It also interacts with the Ktor client pipeline to intercept requests and responses.

Risk Considerations

Potential risks include: 1. Performance overhead due to span creation and breadcrumb generation. 2. Memory issues if response bodies are too large when capturing errors. 3. Thread safety issues if scopes are accessed concurrently. 4. Compatibility issues with future versions of Ktor or the Sentry SDK. 5. Incorrect configuration leading to excessive error reporting or missed errors.

Notable Implementation Details

Notable implementation details include: 1. The use of AttributeKey to store request start timestamps and spans within the Ktor request context. 2. The use of TracingUtils.traceIfAllowed to add Sentry trace headers to requests. 3. The implementation of captureClientError to create and capture Sentry events for failed HTTP requests. 4. The use of forceScopes for testing purposes to bypass the standard scope injection mechanism.

seer-by-sentry[bot]
seer-by-sentry bot reviewed Jul 16, 2025
View reviewed changes
@lcian lcian force-pushed the lcian/feat/ktor-client branch from 28e039a to bb52a1b Compare July 17, 2025 13:03
@lcian lcian force-pushed the lcian/feat/ktor-client branch from 10fa401 to 8a42315 Compare July 17, 2025 13:26
@lcian
Copy link
Member Author

lcian commented Jul 17, 2025 •
edited
Loading

@romtsn Please let me know what you think about this approach: 8a42315 (#4527)
As far as I can tell it's not possible to do this without reflection, as the config property has internal visibility.
I've also added a ProGuard rule that should ensure this works when ProGuard is being used, as we rely on those class/property names.

The approach is still not perfect because if the config block has some side effects, we would trigger them when creating the fake client.
However this seems to be the only approach to do this successfully.
Otherwise I guess another option would be to somehow detect the usage of OkHttp as engine for Ktor and conditionally disable the instrumentation in SAGP, and leave the responsibility to the user to not double instrument when not using SAGP.

As a next step I want to set up an app using ProGuard and SAGP to ensure this works as intended.

@lcian lcian requested a review from romtsn July 17, 2025 13:42
@lcian
Copy link
Member Author

lcian commented Jul 18, 2025 •
edited
Loading

I've set up an Android sample with SAGP and it seems this approach to detect our OkHttp instrumentation doesn't work. As far as I understand, that's because the bytecode manipulation only adds the interceptor when we're in okhttp3.internal.connection.RealCall::getResponseWithInterceptorChain, so it cannot be detected outside of that method.

@lcian
Copy link
Member Author

lcian commented Jul 21, 2025

The OkHttp instrumentation is automatically added by SAGP, but users would have to manually set up this Ktor integration.
So I think a viable option would be to just state very clearly in the docs that if you're using OkHttp as the engine and already using the OkHttp integration or SAGP with auto instrumentation enabled, you should not use the Ktor plugin.
What do you think @romtsn ?

@romtsn
Copy link
Member

romtsn commented Jul 21, 2025

The OkHttp instrumentation is automatically added by SAGP, but users would have to manually set up this Ktor integration. So I think a viable option would be to just state very clearly in the docs that if you're using OkHttp as the engine and already using the OkHttp integration or SAGP with auto instrumentation enabled, you should not use the Ktor plugin. What do you think @romtsn ?

oh dang, sorry I made you go through this. Yeah, this sounds like a job for the gradle plugin to do. Could you file an issue there please? And highlighting this in the docs for now sounds good too 👍

@lcian
Copy link
Member Author

lcian commented Jul 24, 2025

Okay, opened the issue in SAGP.
Tested with a sample app and functionality looks good.
I'm going to prepare docs and then merge this.

adinauer
adinauer approved these changes Jul 24, 2025
View reviewed changes
Copy link
Member

@adinauer adinauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM one question about reported integration name.

sentry-ktor-client/src/main/java/io/sentry/ktorClient/SentryKtorClientPlugin.kt
// Init
SentryIntegrationPackageStorage.getInstance()
.addPackage("maven:io.sentry:sentry-ktor-client", BuildConfig.VERSION_NAME)
addIntegrationToSdkVersion("Ktor")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also have client in the name?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seer-by-sentry seer-by-sentry[bot] seer-by-sentry[bot] left review comments

@adinauer adinauer adinauer approved these changes

@stefanosiano stefanosiano Awaiting requested review from stefanosiano stefanosiano is a code owner

@markushi markushi Awaiting requested review from markushi markushi is a code owner

@romtsn romtsn Awaiting requested review from romtsn romtsn is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@lcian @romtsn @adinauer @getsentry-bot